-
-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cleanup middleware registering #12621
Conversation
bfbc52c
to
30af35d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in general. Left two comments for code that might not be necessary anymore and can be cleaned up while touching it :)
30af35d
to
499668b
Compare
@rullzer what is the status of this one? |
status; waiting for me to have some time and finish it |
499668b
to
4b695a4
Compare
Fixes #12224 Since we only use the middleware at 1 location it makes no sense to register them in each and every container. Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
4b695a4
to
54ff913
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good and works fine 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
) | ||
); | ||
$dispatcher->registerMiddleware( | ||
new CORSMiddleware( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a follow-up PR we could also have these instances constructed by the container if possible 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we could. But it is less of an issue since it is a static list. But agreed for later.
Fixes #12224
Since we only use the middleware at 1 location it makes no sense to
register them in each and every container.
Signed-off-by: Roeland Jago Douma roeland@famdouma.nl